fix(ui): keep loading below pending chat message - #5041
colaforniaw wants to merge 1 commit into
Conversation
d43829c to
5dad8b8
Compare
48c1a4a to
ca9769f
Compare
github-actions
Bot
added
effort/M
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
One P2 remains on this head. The change correctly stops attaching an unidentified running state to the previous settled turn, but it also moves queued follow-ups above the active loading row. Local validation passed: build:test, UI 424/424, the focused UI/Desktop handoff set 27/27, UI typecheck, changed-file Biome, and diff checks. GitHub currently exposes only the green label check for this head; no hosted test check is present. The branch cleanly merges with current main (9cb5cc93c).
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| // Only an identified, materialized live turn may own the inline loading UI. | ||
| // When the host has not exposed a turn yet, keep the status at the transcript | ||
| // boundary instead of guessing that the last loaded turn is still running. | ||
| const tailTurnId = liveInFlight && turns.some((turn) => turn.turnId === props.liveTurn!.turnId) |
There was a problem hiding this comment.
[P2] Keep queued follow-ups below the current response status
When runningStatus is true before a live turn is identified or materialized, this makes tailTurnId undefined. The unchanged rendering below then emits every non-inline transient row before the fallback loading section, including messages whose documented placement is next_turn ("below" the active turn). A render with one current_turn and one next_turn message produced current=5108, next=7732, loading=10156, so the queued follow-up appears above the status for the response that is still running. Please split the fallback ordering so the current pending message remains above loading while next_turn rows remain below it, and add that mixed-placement regression.
6a61c7b to
1756376
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
The previous loading-placement P2 is fixed on this head: the mixed transient path now renders current_turn → loading → next_turn, and the new regression exercises that ordering.
One P2 remains because the newly added test file fails the repository's mandatory ASF header audit. Local validation otherwise passed: build:test, UI 425/425, Desktop 2,361/2,361, focused handoff/queue coverage 42/42, UI typecheck, full lint and format checks, renderer architecture 101/101, changed-file Biome, and git diff --check. npm run check:asf-headers fails with this new file as the sole violation. No hosted checks are currently reported for this head; GitHub reports it mergeable, and it cleanly merges with current main (8d5c4612c46b19270f00fe7aea33c39dff23dbe5).
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| @@ -0,0 +1,117 @@ | |||
| /* | |||
| * Licensed to the Apache Software Foundation (ASF) under one or more | |||
There was a problem hiding this comment.
[P2] Replace this with the repository's canonical ASF header rendering. On this exact head, npm run check:asf-headers exits 1 and identifies this newly added file as the only violation because its line wrapping differs from the current canonical form. That required source-policy gate cannot pass until the header is reconciled; copy the canonical block from a current source file or run the repository's header writer, then rerun the audit.
1756376 to
04f6c6a
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed the rewritten head 04f6c6af98aa88f14627b30f3c4d9635eb2279fc. I found no remaining P0-P3 correctness issue.
The previous mixed transient-row regression remains fixed: when the live turn has not yet been identified, the pending current_turn row renders before the fallback loading row, while queued next_turn rows render after it. Once a materialized live turn is identified, loading remains inside that turn. The added regression coverage exercises both mixed and fallback placements. The prior ASF header finding is also resolved with the repository's canonical header.
Validation on this exact head:
npm run build:test- UI tests: 425/425 passed
- Focused tail-claim tests: 5/5 passed
- Full workspace typecheck and lint
- Renderer architecture checks: 101/101 passed
- ASF header check, changed-file Biome check, and
git diff --check - Clean merge-tree against current
main(8d5c4612c46b19270f00fe7aea33c39dff23dbe5)
There are currently no hosted checks attached to this head. I did not run a browser/Electron visual smoke test; the ordering contract was verified through the production React component's SSR tests.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Generated-by: Codex
04f6c6a to
c907a43
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Built and ran this in a worktree rather than reading it. The authority choice is right — preferring liveTurn over the catalog identity, and skipping recordedRuntimeTurnEnded when liveInFlight, both hold up. Two edges aren't closed yet.
First, one correction to the PR body. The unchecked checklist item says the post-rebase UI build is blocked by "pre-existing UI type errors in unrelated files". It isn't: npm --workspace @maka/ui run build exits 0 here, and the full packages/ui suite is 435/435. What you almost certainly hit is the TS2307: Cannot find module '@maka/core/…' batch (plus the TS7006 implicitly any cascade) that appears when @maka/core's dist hasn't been built — I reproduced exactly that list building @maka/ui first, and it vanished after npm --workspace @maka/core run build. Build order, not baseline errors. npm run lint (3490 files) and biome format (2070) are clean too, so that box can be checked.
The red-green claim is better than stated. Restoring the old tailTurnId / hasRenderedLiveTurn gives 4/5 failures in chat-view-tail-claim plus one in chat-view-empty-compaction, not 3/4 — the one survivor (a live turn outranks a conflicting directory identity) rides the untouched liveInFlight path. Worth noting for its own sake: under the old logic the second case doesn't fail, it OOMs (SIGKILL after 96s), so injecting liveStreaming into an already-settled tail turn was pathological, not merely misplaced.
P2 — An unidentified running turn re-exposes clickable footer actions
Reachability ②.
activeTurnId is undefined whenever runningTurnIds isn't exactly one value — concurrent turns (a sub-agent, a scheduled task, and the user's own turn), or a host activeSession that carries no runningTurnIds at all. Then no TurnView receives liveStreaming, and chat-turn.tsx:751 is actions={props.liveStreaming ? [] : props.footerActions ?? []}. The old code emptied that list by always injecting into the tail turn; the new code has no fallback owner.
Probed with the same messages (t2 carrying settled assistant text, no turn_state):
runningTurnIds |
t2's data-live-streaming |
Regenerate buttons rendered |
|---|---|---|
['t2'] |
true |
1 (t1 only) |
['t2','other'] |
null |
2 |
| absent | null |
2 |
So "review P2-B" — rendering actionable footers on an answer that is still being written, which chat-view.tsx's own comment names as the thing to prevent — comes back on those two branches. editUserMessageDisabled is still held by the global streamingActive, so this is limited to regenerate/branch.
Not a one-line fix: either suppress footerActions on the last turn when streamingActive && activeTurn === undefined, or accept that a degraded owner is still needed when there's no single identity. Please pick one and add a multi-valued runningTurnIds case.
P3 — Three derivations of "which turn is live", and three copies of "recorded as terminal"
Neither is a true parallel authority, but both are the kind of thing the next reader mistakes for a bug.
app-shell.tsx:1491 and use-workhub-controller.ts:364 both compute liveTurn non-terminal ? liveTurn.turnId : runningTurnIds[0]; this PR introduces length === 1 ? [0] : undefined at chat-view.tsx:465. They disagree under concurrency. The questions genuinely differ — where to route a send, versus which row owns the indicator — so I'm not calling it a second authority, but one sentence in the PR body about why the render side is stricter would stop someone "fixing" one of them.
Separately, recordedRuntimeTurnEnded (chat-view.tsx:474-475), turnRecordedAsEnded (materialize.ts:544) and the same pair inline at chat-turn.tsx:469 are now literally identical. Exporting one isRecordedTerminal(turn) from materialize.ts is cheap and is the direct application of 同一 PR 减少概念.
P3 — runningTurnIds being absent is a documented, legal state with no test
packages/core/src/session.ts:390-395 says summaries returned from a mutation omit the field, and shared-session-catalog-projection.ts:47-49 omits it when there's no liveRunState. In that state the new logic degrades to the boundary fallback. I couldn't construct a live desktop path where runningStatus === true and runningTurnIds === undefined simultaneously (deriveTurnActive's two witnesses fail together), so I'm not grading it higher — but pin the degradation with a case.
中文
这次是在 worktree 里实际构建并运行，不是读代码。权威的选择是对的——liveTurn 优先于目录身份，以及 liveInFlight 时跳过 recordedRuntimeTurnEnded，两条都站得住。还有两个边角没收干净。
先更正正文一处。 没打勾的那项 checklist 说 post-rebase 的 UI 构建被"无关文件里既存的类型错误"挡住。不是：在我这里 npm --workspace @maka/ui run build exit 0，packages/ui 全量 435/435。你几乎肯定撞到的是 @maka/core 的 dist 没构建时出现的那批 TS2307: Cannot find module '@maka/core/…'（连带 TS7006 implicitly any 的级联）——我先构建 @maka/ui 时复现了完全相同的列表，先跑 npm --workspace @maka/core run build 之后就全部消失。是构建顺序，不是基线错误。npm run lint（3490 文件）和 biome format（2070 文件）也干净，所以那一项可以勾上。
红绿的实际情况比正文写的更好。把旧的 tailTurnId / hasRenderedLiveTurn 换回去，chat-view-tail-claim 是 4/5 失败，外加 chat-view-empty-compaction 一条，不是 3/4——唯一存活的那条（a live turn outranks a conflicting directory identity）走的是未改动的 liveInFlight 路径。另外值得单独一提：旧逻辑下第二个用例不是断言失败而是 OOM（96 秒后 SIGKILL），说明把 liveStreaming 注进一个已结算的 tail turn 本身就是病态行为，不只是位置不对。
P2 — 未被识别的运行中 turn 会重新露出可点击的 footer action（可达②）
只要 runningTurnIds 不是恰好一个值，activeTurnId 就是 undefined——并发 turn（子 agent、定时任务和用户自己的 turn 同时在跑），或者宿主给的 activeSession 根本不带 runningTurnIds。这时没有任何 TurnView 拿到 liveStreaming，而 chat-turn.tsx:751 是 actions={props.liveStreaming ? [] : props.footerActions ?? []}。旧代码靠"永远注进 tail turn"把这串 action 清空，新代码没有兜底所有者。
用同一批消息实测（t2 有已落盘的 assistant 文本、无 turn_state）：['t2'] → t2 的 data-live-streaming 为 true，渲染 1 个 Regenerate；['t2','other'] 和不提供该字段 → data-live-streaming 为 null，渲染 2 个。
也就是 chat-view.tsx 自己注释里点名要防的 "review P2-B"（在仍在书写的答案上渲染可操作 footer）在这两个分支下回来了。editUserMessageDisabled 仍由全局 streamingActive 挡着，所以只影响 regenerate/branch 一类。
修法不是一行：要么在 streamingActive && activeTurn === undefined 时对最后一个 turn 也压掉 footerActions，要么承认"没有唯一身份"时仍需要一个降级所有者。请明确选一个，并补一条 runningTurnIds 为多值的用例。
P3 — "哪个 turn 是活的"有三种推导，"已记录为终态"有三份拷贝
两者都不算真正的并行权威，但都属于下一个读代码的人会误认为是 bug 的那类。
app-shell.tsx:1491 和 use-workhub-controller.ts:364 都算 liveTurn 非终态 ? liveTurn.turnId : runningTurnIds[0]；本 PR 在 chat-view.tsx:465 引入的是 length === 1 ? [0] : undefined。并发时两者给出不同答案。它们服务的问题确实不同——发送往哪路由，和哪一行拥有指示器——所以我不判成第二个权威，但在正文里写一句为什么渲染侧要更严格，能阻止有人去"修"其中一处。
另外，recordedRuntimeTurnEnded（chat-view.tsx:474-475）、turnRecordedAsEnded（materialize.ts:544）和 chat-turn.tsx:469 里同一对判断现在字面完全相同。从 materialize.ts 导出一个 isRecordedTerminal(turn) 成本很低，也是"同一 PR 减少概念"的直接适用点。
P3 — runningTurnIds 缺席是有文档的合法状态，却没有测试
packages/core/src/session.ts:390-395 明说 mutation 返回的 summary 不带该字段，shared-session-catalog-projection.ts:47-49 在没有 liveRunState 时也整个省略。此时新逻辑退化为边界 fallback。我没能在 desktop 上构造出 runningStatus === true 同时 runningTurnIds === undefined 的活路径（deriveTurnActive 的两个证人会连带失效），所以不升级——但请补一条用例把这个降级钉住。
| // legacy status does not supply that evidence; a live projection still wins. | ||
| const recordedRuntimeTurnEnded = !liveInFlight | ||
| && identifiedTurn?.statusSource === 'recorded' && identifiedTurn.status !== 'running'; | ||
| const streamingActive = !recordedRuntimeTurnEnded && ( |
There was a problem hiding this comment.
P2 (reachability ②) A stale recorded terminal turns the indicator off entirely, including under a freshly sent prompt that hasn't materialized yet.
The path: t1 completes → the transcript records turn_state completed → the catalog lags and still reports runningTurnIds: ['t1'] → the user sends the next message, the transient bubble is on screen, liveTurn hasn't armed. recordedRuntimeTurnEnded is true → streamingActive is false → nothing renders under the pending message, and ChatMessageList's isStreaming goes false with it.
Probed (runningStatus: true, t1 with a recorded completed state, one transient at transientPlacement: 'current_turn'):
S2 stale recorded + pending send: statuses=0 at=[] boundaryFallbackSection=false
S6 stale recorded + newer history: statuses=0 at=[] boundaryFallbackSection=false
The old code put the indicator on the wrong (completed) turn here; the new code removes it altogether — the same class of problem this PR set out to kill, in a different shape. recordedRuntimeTurnEnded should invalidate the stale identity, not veto streamingActive while local pending evidence exists:
const staleIdentity = !liveInFlight
&& identifiedTurn?.statusSource === 'recorded' && identifiedTurn.status !== 'running';
const hasPendingCurrentTurnSend = transientMessages.some(
(message) => message.transientPlacement === 'current_turn',
);
const streamingActive = (!staleIdentity || hasPendingCurrentTurnSend) && (
liveInFlight || (!props.liveTurn?.terminal && !!props.runningStatus && !isCompactionLive)
);
const activeTurnId = streamingActive && !staleIdentity ? identifiedTurnId : undefined;
const activeTurn = streamingActive && !staleIdentity ? identifiedTurn : undefined;With no transient the behaviour is unchanged — I checked that recorded terminal evidence outranks a stale runtime ID carries no transient, so it still passes. With one, the identity is stripped and the indicator lands in the boundary fallback under the pending message, which is what the PR says it wants. Please add a "stale recorded identity + pending prompt" case.
中文
（可达②）陈旧的 recorded 终态会把指示器整个关掉，连刚发出、还没物化的新 prompt 下面也没有。
路径：t1 完成 → transcript 落了 turn_state completed → 目录刷新滞后，仍报 runningTurnIds: ['t1'] → 用户立刻发下一条，transient 气泡上屏，liveTurn 还没 arm。此时 recordedRuntimeTurnEnded 为真 → streamingActive 为假 → 待发消息下面一个指示器都没有，ChatMessageList 的 isStreaming 也跟着变假。
实测（runningStatus: true，t1 带 recorded completed，一条 transientPlacement: 'current_turn' 的 transient）见上方输出。
旧代码在这里是把指示器错贴到已完成的 t1 上；新代码是让它彻底消失——正是本 PR 想消灭的那类问题换了个形态。recordedRuntimeTurnEnded 应该只作废那个陈旧的身份，而不是在存在本地待发证据时连 streamingActive 一起否决（改法见上）。
无 transient 时行为不变——我核对过 recorded terminal evidence outranks a stale runtime ID 这条用例不带 transient，仍然通过。有 transient 时身份被剥掉、指示器落到待发消息下方的 boundary fallback，正是 PR 声明的目标。请补一条"陈旧 recorded 身份 + 待发 prompt"的用例。
| const pendingRunningStartedAt = transientMessages.findLast((message) => | ||
| message.transientPlacement === 'current_turn' | ||
| && (tailTurnId === undefined || message.hostTurnId === undefined || message.hostTurnId === tailTurnId), | ||
| && (activeTurnId === undefined || message.hostTurnId === undefined || message.hostTurnId === activeTurnId), |
There was a problem hiding this comment.
P3 When activeTurnId is undefined, pendingRunningStartedAt picks the last transient regardless of hostTurnId. With two pending prompts under concurrency (or an empty runningTurnIds), the clock starts from the wrong one. Use find rather than findLast in that branch, or accept only the transient whose hostTurnId is undefined.
中文
activeTurnId 为 undefined 时，pendingRunningStartedAt 会挑最后一条 transient，无视 hostTurnId。并发下有两条待发 prompt（或 runningTurnIds 为空）时，计时器会从错的那条起算。在该分支改用 find 而不是 findLast，或者只接受 hostTurnId === undefined 的那条。
| return renderToStaticMarkup(createElement(LocaleProvider, { | ||
| locale: 'en', | ||
| children: createElement(ChatSurfaceLayout, { composer: null, children: view }), | ||
| })); |
There was a problem hiding this comment.
P3 This assertion depends on JSX attribute order. assert.doesNotMatch(markup, /data-turn-id="t1"[^>]*data-live-streaming="true"/) only means anything while chat-turn.tsx:486-487 keeps data-turn-id before data-live-streaming; swap those two lines and the assertion silently becomes vacuously true. Same at :108-109 and :130-131, and the markup.indexOf(...) ordering comparisons have the same problem.
The third case in this very file already does it right — copy that:
assert.equal(
document.querySelector('section[data-turn-id="t1"]')?.getAttribute('data-live-streaming'),
null,
);中文
这条断言依赖 JSX 属性的书写顺序。assert.doesNotMatch(markup, /data-turn-id="t1"[^>]*data-live-streaming="true"/) 只在 chat-turn.tsx:486-487 保持 data-turn-id 先于 data-live-streaming 时才有意义；一旦有人调换这两行，断言会静默变成永真。:108-109 和 :130-131 同理，markup.indexOf(...) 的顺序比较也有同样问题。
同一个文件的第三个用例已经写对了，照抄即可（见上）。
| // interned by value to line up again. | ||
| const turnPresentation = props.deriveTurnPresentation?.(turns); | ||
| // #642 single render path: the in-flight answer is injected into the tail | ||
| // #642 single render path: the in-flight answer is injected into its own |
There was a problem hiding this comment.
P3 Two comment leftovers. The edit here breaks mid-sentence — // Only the identified TurnView gets a fresh ends the line — and reads like a lost half. And at :934, "Mutually exclusive with the tail injection above" refers to a tail injection that no longer exists; it's the identified turn now.
中文
两处注释残留。这里的改动断在半句上——// Only the identified TurnView gets a fresh 单独成行——读起来像漏了半行。另外 :934 的 "Mutually exclusive with the tail injection above" 指向的 tail injection 已经不存在了，现在是 identified turn。
3 participants
Summary
When a new chat message was visible as a transient row but had not yet been persisted into
turns, the running status could attach to the previous completed turn.This change keeps the running status below the pending message until the new turn is materialized. When a live turn identity is available and its turn is already materialized, the status remains inside that turn. It also avoids claiming the last loaded turn while newer history is unavailable. In that fallback, the transcript boundary is no longer marked as the active row until a materialized live turn is available.
Added regression coverage for:
Verification
git diff --checkpassed.npm run devpassed the libraries, preload, filesystem worker, and main builds, then started Vite and Electron successfully.npm --workspace @maka/ui run typecheckpassed before this rebase.npm --workspace @maka/ui run buildpassed before this rebase; after rebasing, the current worktree is blocked by pre-existing UI type errors in unrelated files.node --test packages/ui/dist/__tests__/chat-view-tail-claim.test.jspassed: 5/5 after rebuilding with the current source (the workspace build still reports unrelated baseline type errors).npm run lintpassed: 3,419 files checked.npm run format:checkpassed: 2,020 files checked.AI use
Tool(s) and scope: OpenAI Codex analyzed the race condition, implemented the UI fix, and added regression tests.
Checklist
Tests cover the change and fail without it
Lint, format, typecheck and the affected suites pass locally
The affected source files pass Biome and
git diff --check; the post-rebase UI build is blocked by unrelated baseline type errors.Does this PR entail a change in behavior?